Update RMS aggregator with lazy function#3130
Merged
corinnebosley merged 6 commits intoSciTools:masterfrom Oct 24, 2018
Merged
Conversation
stickler-ci
reviewed
Aug 3, 2018
| def test_masked(self): | ||
| # Masked entries should be completely ignored. | ||
| data = as_lazy_data(ma.array([5, 10, 2, 11, 6, 4], | ||
| mask=[False, True, False, True, False, False], |
There was a problem hiding this comment.
E128 continuation line under-indented for visual indent
pelson
approved these changes
Aug 10, 2018
lib/iris/analysis/__init__.py
Outdated
| # However `da.average` current doesn't handle masked weights correctly | ||
| # (see https://github.com/dask/dask/issues/3846). | ||
| # To work around this we use da.mean, which doesn't support weights at | ||
| # all, so we raise an error rather than silently giving the wrong answer. |
Member
There was a problem hiding this comment.
Where is the error being raised? Is that caused by passing invalid kwargs through to da.mean?
Member
Author
There was a problem hiding this comment.
@pelson apologies, that comment isn't that clear on re-reading. But you're right, the error comes from da.mean receiving a bad kwarg:
TypeError: mean() got an unexpected keyword argument 'weights'
I'll improve the wording of the comment.
corinnebosley
approved these changes
Oct 24, 2018
Member
corinnebosley
left a comment
corinnebosley
left a comment
There was a problem hiding this comment.
@dkillick This is very lovely. Your comments are informative and helpful; your tests are clean, simple and well structured, good job all round.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a lazy aggregation function to the RMS aggregator, which makes use of dask functionality to build the RMS equation.
Note that currently this does not allow lazy weighted RMS aggregation when the input is also masked. This is in place because there appears to be a limitation in
da.average, which means the weights array is not masked to match the input data.Ping @niallrobinson - with thanks for raising the fact the RMS aggregator wasn't lazy, and for pairing on this update.